Skip to content

Fix MO refundable EITC reform crash and zero-refund bug#8642

Open
PavelMakarchuk wants to merge 5 commits into
mainfrom
fix-mo-refundable-eitc-reform-crash
Open

Fix MO refundable EITC reform crash and zero-refund bug#8642
PavelMakarchuk wants to merge 5 commits into
mainfrom
fix-mo-refundable-eitc-reform-crash

Conversation

@PavelMakarchuk

@PavelMakarchuk PavelMakarchuk commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Fixes #8640

Problem

Triggering the bundled Missouri refundable-EITC contrib reform (gov.contrib.states.mo.child_poverty_impact_dashboard.eitc.in_effect) crashed the simulation. After working around the crash, the reform also failed to actually make the WFTC refundable for the low-liability filers refundability is meant to help.

Three bugs fixed in mo_refundable_eitc_reform.py

1. add() received a parameter path string (crash). mo_refundable_credits.formula passed the literal string "gov.states.mo.tax.income.credits.refundable" to add(), which iterates it character by character — get_variable("g") returns None → AttributeError: 'NoneType' object has no attribute 'entity'. Now resolves the parameter to its list of variable names first.

2. Mixed computation modes (crash on core ≥3.26.8). mo_refundable_credits redeclared a formula but inherited adds from the baseline variable, tripping the engine's strict mode check (ValueError: ... mixes computation modes). Now sets adds = None and subtracts = None explicitly.

3. Refundable credit capped at tax liability (silent zero-refund). mo_refundable_wftc returned mo_wftc — the applied credit capped at MO tax liability — so a zero-liability filer received $0 even though refundability should pay the full amount. Now uses mo_wftc_potential (the uncapped credit). This is the functional fix that makes the reform do what it claims.

Verification

Reproduced the issue's exact script — runs without raising. Verified the reform's effect against baseline for a Missouri single parent, 2 kids, $20k earnings, 2026 (federal EITC $7,316 → potential WFTC $1,463):

Variable Baseline Reform
mo_refundable_credits $0 $1,463.20
mo_income_tax $0 −$1,463.20 (refund)
household_net_income $36,103.81 $37,567.01

Also confirmed no double-counting at positive liability: a moderate-income case yields a delta exactly equal to the previously-lost (nonrefundable-capped) portion of the credit.

Tests

  • New tests/policy/reform/mo_refundable_eitc.yaml: verifies the full potential WFTC is paid as refundable at zero liability, the nonrefundable portion is zeroed, and there's no double-counting when liability already absorbs the credit.
  • All 361 Missouri baseline tests still pass.

🤖 Generated with Claude Code

PavelMakarchuk and others added 3 commits June 15, 2026 18:31
@PavelMakarchuk @claude
Fix MO refundable EITC reform crash and zero-refund bug
112bd2e 
The bundled Missouri refundable-EITC contrib reform crashed at calculation
time and, once running, paid no refundable credit to zero-liability filers.
Three issues fixed in mo_refundable_eitc_reform.py:

1. mo_refundable_credits passed a parameter path string to add(), which
   iterates it character by character and raises AttributeError. Resolve
   the parameter to its list of variable names first.
2. mo_refundable_credits redeclared a formula but inherited adds from the
   baseline, tripping the core engine's mixed computation-mode check. Set
   adds = None and subtracts = None explicitly.
3. mo_refundable_wftc returned mo_wftc, the credit capped at tax liability,
   so the reform paid $0 refundable for low-liability filers. Use
   mo_wftc_potential so the full credit is refundable.

Fixes #8640

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@PavelMakarchuk @claude
Address review: strengthen MO reform test assertions
c180b26 
- Case 1 now asserts mo_wftc: 0 (capped) alongside mo_refundable_wftc: 1000
  (uncapped), so the capped-vs-uncapped contrast that is the actual fix is
  guarded; reverting the fix would now fail the test.
- Case 2 now asserts mo_income_tax: 0 and mo_income_tax_before_refundable_
  credits: 1000, genuinely verifying the credit offsets liability exactly
  once (no double counting) rather than re-asserting case 1's outputs.
- Fix grammar in the mo_refundable_wftc comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@PavelMakarchuk @claude
Update pre-existing MO reform tests for uncapped refundable WFTC
75ce33d 
The refundability fix (mo_refundable_wftc now reads mo_wftc_potential) broke
two pre-existing tests that encoded the old capped behavior:

- The contrib reform test injected mo_wftc directly and asserted the refundable
  amount equaled it. Rewrite it to drive the credit from the federal EITC and
  assert the full uncapped potential is paid (with mo_wftc capped to 0 at zero
  liability), which is what the fix corrects.
- The applied-credit downstream-consumer guard listed the reform as a consumer
  of the applied mo_wftc. The reform now consumes mo_wftc_potential instead -
  exactly the pattern the guard recommends - so drop the stale entry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@PavelMakarchuk PavelMakarchuk requested a review from DTrim99 June 16, 2026 01:15
This was referenced Jun 16, 2026
Open
Open
@DTrim99

DTrim99 commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

PR Review — #8642: Fix MO refundable EITC reform crash and zero-refund bug

A focused bug fix to the Missouri refundable-EITC contrib reform (mo_refundable_eitc_reform.py), fixing #8640. Reviewed for correctness, code patterns, test coverage, and CI. All three claimed fixes were independently verified as correct — no silently dropped credits and no double-counting.

🔴 Critical (Must Fix)

None. The fixes are correct and the PR is merge-ready on the code. (The one failing CI job is unrelated infrastructure flakiness — see below — not a PR defect.)

🟡 Should Address

  1. Add a multi-refundable-credit regression test (highest value). Bug 1's fix rewrote how the non-WFTC refundable credits are summed (resolving the refundable parameter to its variable list, currently ["mo_property_tax_credit"], then add()-ing it alongside mo_refundable_wftc). No test injects a second MO refundable credit, so a future regression that drops other_refundable (returning only WFTC) would pass silently. Add a case with a property-tax credit present and assert it survives.
  2. Currency test cases omit absolute_error_margin: 0.01. Both new/changed YAML tests use exact-integer expectations; adding the one-cent margin is the convention and guards against float drift.
  3. Forward-compat fragility (worth a code comment). The reform's mo_non_refundable_credits override returns only mo_non_refundable_wftc, discarding the baseline's ordered credit-cap summation over the full nonrefundable list. Today that list is just [mo_wftc] so behavior is identical — but if MO later adds another nonrefundable credit, the reform would silently zero it. This is pre-existing in the base file (not introduced by this diff); a comment (or summing the full list) would prevent a future surprise.

🟢 Suggestions

  1. Add boundary cases in the reform-level test: a zero-EITC filer (potential WFTC = $0) and a partial-liability case (liability between 0 and the potential credit) to lock down the capped-vs-uncapped boundary.
  2. Align test case names to the Case N, description. convention.
  3. Optional: the Bug-2 "mixes computation modes" ValueError only triggers on core ≥3.26.8 (the installed core 3.26.0 silently ignores inherited adds when a formula exists). The adds = None/subtracts = None change is still the correct forward-compatible pattern — just noting the crash actually fixed on current core is Bug 1 (the string path).

Findings detail

  • Bug 1 (string passed to add() → crash): Verified. add() iterates its 3rd arg as variable names; a bare parameter-path string iterates char-by-char → get_variable("g") → AttributeError. The fix resolves the refundable parameter to its list first, then calls add(). Idiomatic and period-correct.
  • Bug 2 (adds inherited + custom formula): Correct. update_variable retains inherited attributes; setting adds = None/subtracts = None is the right pattern. Completeness confirmed — the reform's formula reproduces the full baseline refundable set (mo_property_tax_credit) plus mo_refundable_wftc; nothing dropped.
  • Bug 3 (capped mo_wftc → $0 at zero liability): Correct. mo_wftc_potential is the uncapped credit (eitc × wftc.match); returning it is right for refundability. No double-counting — the reform also overrides mo_non_refundable_credits to drop WFTC from the nonrefundable side, so it's counted exactly once. The regression test (eitc=5000, before_credits=1000 → mo_income_tax=0, not negative) confirms this.
  • Code-health test change is legitimate, not a weakening. Switching consumption from the capped mo_wftc to uncapped mo_wftc_potential removes the exact anti-pattern test_non_refundable_credit_downstream_consumers.py guards, so the allowlist entry must be removed or the test fails. It strengthens pattern adherence.
  • Baseline unaffected: …eitc.in_effect defaults false; reform factory returns None and overrides nothing when off.

Validation Summary

Check Result
Reform correctness (3 fixes) PASS — all correct, no dropped credits, no double-counting
Code patterns PASS — idiomatic; code-health test change legitimate
Test coverage Adequate — all 3 bugs guarded; add multi-credit + boundary cases
References N/A — no parameters/law values in this PR
CI Status 1 job failed (Contrib states-shard-2) — infrastructure timeout during an unrelated RI batch; the PR's own MO test passed in that job. Re-run to clear.

Review Severity: COMMENT

No blocking issues — the fixes are verified correct. Recommendations are test-coverage enhancements and one pre-existing forward-compat note.

Next Steps

  • Re-run the cancelled Contrib (states-shard-2) job (infrastructure, not code).
  • To apply the suggested test additions: /fix-pr 8642.

This was referenced Jun 17, 2026
Open
Open
@PavelMakarchuk @claude
Address PR review: multi-credit test, boundary cases, forward-compat …
2fdc923 
…note

DTrim99 review on #8642:
- Add a multi-refundable-credit regression test (mo_property_tax_credit
  pinned to a positive value) that proves the reform's mo_refundable_credits
  formula sums all entries in `gov.states.mo.tax.income.credits.refundable`
  alongside mo_refundable_wftc rather than replacing them. Closes the
  silent-regression gap the original tests didn't cover.
- Add zero-EITC boundary case (potential WFTC = 0) and partial-liability
  case (liability=400, potential=1,000) to pin down the capped-vs-uncapped
  refund boundary; the partial case asserts mo_income_tax=-600 (the
  refund flowing through), guarding against a future double-count
  regression that would land at -1,000.
- Add `absolute_error_margin: 0.01` to every currency assertion in the
  new reform-test yaml, matching project convention.
- Add forward-compat comment on the reform's mo_non_refundable_credits
  formula: today the bucket is just `[mo_wftc]` so the simplified
  formula is equivalent to the baseline, but if MO adds a second
  nonrefundable credit later this would silently drop it — at that
  point switch to the UT/OH pattern (ordered-cap walk with mo_wftc
  filtered out).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@PavelMakarchuk PavelMakarchuk requested a review from hua7450 June 17, 2026 03:33
@hua7450 hua7450 marked this pull request as ready for review June 17, 2026 13:49
@PavelMakarchuk PavelMakarchuk mentioned this pull request Jun 17, 2026
Draft
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hua7450 hua7450 hua7450 approved these changes

@DTrim99 DTrim99 Awaiting requested review from DTrim99

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MO refundable EITC reform (create_mo_refundable_eitc) crashes when triggered

3 participants

@PavelMakarchuk @DTrim99 @hua7450